-
Notifications
You must be signed in to change notification settings - Fork 744
Objective-C interop doc updates #6942
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Visit the preview URL for this PR (updated for commit af0282f): |
|
I believe the failure was from a version of Jaspr that had a regression. I updated the branch with main which should pull in the release with a fix.
You're right, Swift isn't supported yet, but if I'm planning to work on basic support for Swift soon, is the no highlighting fallback sufficient until I can finish that? |
Yep, no hilighting is fine for now. |
|
@parlough This is ready for review. PTAL |
|
Sorry about the delay in reviewing this. I'll make sure to take a look before Monday! \cc @antfitch For reviewing the writing and structure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall these updates look good to me after a few updates, thanks for coming back to them @liamappelbe!
Do wait for @antfitch's approval as well before landing :)
No need for me this time. Triaging this back to you @parlough to land. Love the changes, folks! |
|
Hey all, this is a bit larger than I first thought. @parlough, when you're finished with your review, let me know and I'll do a final edit pass. |
antfitch
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After the engineering pass with @parlough, I'll circle by with a writing pass.
Co-authored-by: Parker Lougheed <[email protected]>
Co-authored-by: Parker Lougheed <[email protected]>
Co-authored-by: Parker Lougheed <[email protected]>
Co-authored-by: Parker Lougheed <[email protected]>
Co-authored-by: Parker Lougheed <[email protected]>
Co-authored-by: Parker Lougheed <[email protected]>
Co-authored-by: Parker Lougheed <[email protected]>
Co-authored-by: Parker Lougheed <[email protected]>
Co-authored-by: Parker Lougheed <[email protected]>
Co-authored-by: Parker Lougheed <[email protected]>
Co-authored-by: Parker Lougheed <[email protected]>
Co-authored-by: Parker Lougheed <[email protected]>
Co-authored-by: Parker Lougheed <[email protected]>
Co-authored-by: Parker Lougheed <[email protected]>
Co-authored-by: Parker Lougheed <[email protected]>
Co-authored-by: Parker Lougheed <[email protected]>
Added my edits directly to the Objective-C interop documentation. Updated some wording and changed headings to active voice. Added heading anchors to preserve backwards compatibility with heading changes.
antfitch
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added my edits directly to objective-c-interop.md. Nothing significant and everything looked great.
|
It seems Amanda's commit accidentally undid some of the changes you applied and made. Tomorrow I can reapply what's necessary and land this. Thank you both! |
| output: Output( | ||
| dartFile: Uri.file('avf_audio_bindings.dart'), | ||
| preamble: ''' | ||
| // ignore_for_file: camel_case_types, non_constant_identifier_names, unused_element, unused_field, void_checks, annotate_overrides, no_leading_underscores_for_local_identifiers, library_private_types_in_public_api |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@liamappelbe Do we actually need to specify this ignore_for_file here? It looks like FFIgen automatically generates // ignore_for_file: type=lint, which disables all these lints already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure. This code is the same as what I have in ffigen/example/objective_c, and I needed it there to avoid analysis issues on CI. But maybe you don't need it when you're doing this stuff locally. Does dart analyze run without errors/warnings in your repo?
I tried out the revamped Objective-C interop instructions from #6942 (thank you for updating them 🙏 ) and noticed a handful of minor issues that this PR addresses: * Instruct people to add dependencies to the helpers `package:objective_c` and `package:ffi`. * Add a missing import to `package:objective_c` to the sample code to resolve `NSString`. * Adjusted some file paths to align with best practices (e.g. generate the bindings into the `lib` directory). * Fixed a nullability issue by checking `player == null` and bailing early. * ~Fixed an issue with the enumeration in the multithreading section.~ Already fixed in #6995. * Removed the `ignore_for_file` preamble configuration since FFIgen auto-generates `// ignore_for_file: type=lint`. I also changed the second half to use highlights in the sample code just like it is done in the first half. I find that easier for following along. Plus, in the end you get to see the complete code. I've also incorporated #6990 into this PR.
Update The Objective-C interop documentation to use the new FFIgen config Dart API, instead of YAML configs. I haven't updated the Swift example, because that will soon be replaced by swiftgen.
Fixes dart-lang/native#2710